feat(server/v2): Add Swagger UI support for server/v2 #23092
Conversation
crStiv
requested review from
julienrbrt,
hieuvubk,
kocubinski and
a team
as code owners
|
Warning Rate limit exceeded@crStiv has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 40 minutes and 53 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe pull request introduces a new Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
C:server/v2
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/v2/api/swagger/config.go(1 hunks)server/v2/api/swagger/handler.go(1 hunks)server/v2/server.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
server/v2/api/swagger/handler.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/api/swagger/config.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
server/v2/api/swagger/handler.go
44-44: undefined: time
(typecheck)
server/v2/api/swagger/config.go
24-24: undefined: fmt
(typecheck)
server/v2/server.go
78-78: undefined: http
(typecheck)
88-88: undefined: http
(typecheck)
251-251: s.config.API undefined (type ServerConfig has no field or method API)
(typecheck)
🔇 Additional comments (5)
server/v2/api/swagger/config.go (2)
5-11: Struct fields look good
Your Config struct is straightforward and clearly documents each field. This aligns well with Go style guidelines.
13-19: Clarity in default values
Returning Enable: false and Path: "/swagger" as defaults appears consistent with the code’s intended usage. This ensures that Swagger UI is turned off by default unless explicitly enabled.
server/v2/api/swagger/handler.go (1)
12-46: Handler logic and MIME handling
Your approach to handle file serving, MIME type detection, and fallback to index.html is solid. The usage of path.Clean is sensible for preventing tricky path manipulations. Once the time package is imported, http.ServeContent is a good fit for robust file serving.
🧰 Tools
🪛 golangci-lint (1.62.2)
44-44: undefined: time
(typecheck)
server/v2/server.go (2)
Line range hint 78-88: HTTP router usage
Defining router *http.ServeMux and assigning it with http.NewServeMux() is sensible for isolating routes. Confirm the design pattern for hooking in additional routes from other components if needed.
🧰 Tools
🪛 golangci-lint (1.62.2)
78-78: undefined: http
(typecheck)
249-263: Potential config mismatch
Line 251 references s.config.API.Swagger, but the static analysis suggests that ServerConfig has no field API. Verify your ServerConfig type to ensure it holds the API field or reference the correct nested structure.
🧰 Tools
🪛 golangci-lint (1.62.2)
251-251: s.config.API undefined (type ServerConfig has no field or method API)
(typecheck)
server/v2/api/swagger/config.go
Outdated
| // Validate validates the configuration | ||
| func (c Config) Validate() error { | ||
| if c.Path == "" { | ||
| return fmt.Errorf("swagger path cannot be empty") | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Ensure error formatting is enabled
While the validation logic is correct, remember that fmt.Errorf("swagger path cannot be empty") requires "fmt" to be imported (as noted above). Once the import is added, this code works as intended.
🧰 Tools
🪛 golangci-lint (1.62.2)
24-24: undefined: fmt
(typecheck)
server/v2/api/swagger/config.go
Outdated
| package swagger | ||
|
|
||
| import "github.com/cosmos/cosmos-sdk/server/v2/config" | ||
|
|
There was a problem hiding this comment.
Package import oversight
Currently, you're importing "github.com/cosmos/cosmos-sdk/server/v2/config" but using fmt.Errorf in the code without importing "fmt". There's no direct usage of the imported "github.com/cosmos/cosmos-sdk/server/v2/config" in this snippet. If it's needed in other parts of the file, that's fine. Otherwise, consider adding the missing "fmt" import or removing unused imports.
package swagger
-import "github.com/cosmos/cosmos-sdk/server/v2/config"
+import (
+ "fmt"
+ "github.com/cosmos/cosmos-sdk/server/v2/config"
+)
Committable suggestion skipped: line range outside the PR's diff.
server/v2/api/swagger/handler.go
Outdated
| import ( | ||
| "net/http" | ||
| "path" | ||
| "path/filepath" | ||
| "strings" | ||
|
|
||
| "github.com/rakyll/statik/fs" | ||
| ) |
There was a problem hiding this comment.
Missing time import
You're calling time.Time{} in this file but haven't imported "time". Add the missing import to avoid compilation issues.
import (
"net/http"
"path"
"path/filepath"
"strings"
"github.com/rakyll/statik/fs"
+ "time"
)Committable suggestion skipped: line range outside the PR's diff.
server/v2/server.go
Outdated
| "github.com/cosmos/cosmos-sdk/server/v2/api/swagger" | ||
| "github.com/pelletier/go-toml/v2" | ||
| "github.com/spf13/cobra" | ||
| "github.com/spf13/pflag" | ||
|
|
||
| "cosmossdk.io/core/transaction" | ||
| "cosmossdk.io/log" | ||
| "github.com/rakyll/statik/fs" |
There was a problem hiding this comment.
Check for missing imports
You reference swagger and statik/fs but also rely on http.ServeMux later (lines 78, 88) without importing "net/http". Ensure all required imports are present to avoid compilation errors.
import (
"context"
"errors"
"fmt"
"os"
"path/filepath"
"strings"
+ "net/http"
"github.com/cosmos/cosmos-sdk/server/v2/api/swagger"
"github.com/pelletier/go-toml/v2"
"github.com/spf13/cobra"Committable suggestion skipped: line range outside the PR's diff.
server/v2/server.go
Outdated
There was a problem hiding this comment.
Hi, thanks for tackling this. The wiring shouldn't be done in the main server, but api/swagger should be its own server component. Check out the other servers.
There was a problem hiding this comment.
Hi, thanks for tackling this. The wiring shouldn't be done in the main server, but api/swagger should be its own server component. Check out the other servers.
oh you're absolutely right, gonna fix it soon!
There was a problem hiding this comment.
@julienrbrt sorry for mess in commits, I had to redo some things but now everything should be fine
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/v2/api/swagger/config.go (1)
4-4: Remove unused imports.The import
"cosmossdk.io/core/server"is not needed and can be safely removed to adhere to Go’s best practices on unused imports.Here’s a suggested fix:
import ( - "cosmossdk.io/core/server" )🧰 Tools
🪛 golangci-lint (1.62.2)
4-4: "cosmossdk.io/core/server" imported and not used
(typecheck)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/v2/api/swagger/config.go(1 hunks)server/v2/api/swagger/server.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
server/v2/api/swagger/config.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/api/swagger/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
server/v2/api/swagger/config.go
4-4: "cosmossdk.io/core/server" imported and not used
(typecheck)
server/v2/api/swagger/server.go
31-31: srv.Config undefined (type *Server[T] has no field or method Config, but does have field config)
(typecheck)
40-40: undefined: NewSwaggerHandler
(typecheck)
server/v2/api/swagger/server.go
Outdated
| srv.config = serverCfg | ||
|
|
||
| mux := http.NewServeMux() | ||
| mux.Handle(srv.config.Path, NewSwaggerHandler()) |
There was a problem hiding this comment.
Handle the undefined NewSwaggerHandler reference.
The function NewSwaggerHandler() is not defined in the reviewed code, nor is it imported from any package. Ensure that NewSwaggerHandler, or whichever handler function you’re calling, is properly defined and imported to prevent a runtime error or undefined symbol.
Would you like assistance creating a NewSwaggerHandler() function?
🧰 Tools
🪛 golangci-lint (1.62.2)
40-40: undefined: NewSwaggerHandler
(typecheck)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| // Set caching headers | ||
| w.Header().Set("Cache-Control", "public, max-age=31536000") | ||
| w.Header().Set("Last-Modified", time.Now().UTC().Format(http.TimeFormat)) |
Check warning
Code scanning / CodeQL
Calling the system time Warning
| w.Header().Set("Last-Modified", time.Now().UTC().Format(http.TimeFormat)) | ||
|
|
||
| // Serve the file | ||
| http.ServeContent(w, r, urlPath, time.Now(), file.(io.ReadSeeker)) |
Check warning
Code scanning / CodeQL
Calling the system time Warning
julienrbrt
changed the title
server/v2/api/swagger/handler.go
Outdated
| ) | ||
|
|
||
| // Handler returns an HTTP handler for Swagger UI | ||
| func Handler() http.Handler { |
There was a problem hiding this comment.
nit: let's not expose this and just use swaggerHandler{} in the handler registration
There was a problem hiding this comment.
The swagger ui is passed nowhere, so this server will actually do nothing.
We should let the user pass its chain swagger ui / file at server instantiation.
For instance, a user could pass the default SDK swagger config (docs.SwaggerUI):
Line 19 in b6150ec
|
Additionally, let's wire this server in simapp/v2 so we can test it out and be shown to users how it is wired. |
@julienrbrt hey Julien, I've added some changes, is it what you asked for? |
Yes great, if you can create a doc.go showing how to wire this server/v2 and update simapp/v2 to wire it, then it looks good to me. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
server/v2/api/swagger/server.go (1)
27-33: Consider Enabling Concurrency or Non-Blocking StartupCurrently,
ListenAndServe()is called directly, which blocks execution until the server shuts down or returns an error. If you need the application to continue executing other tasks, consider running the server in a goroutine or using a more sophisticated concurrency model.func (s *Server[T]) Start(ctx context.Context) error { if !s.config.Enable { s.logger.Info(fmt.Sprintf("%s server is disabled via config", s.Name())) return nil } - if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed { + go func() { + if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed { + s.logger.Error("failed to start swagger server", "error", err) + } + }() return nil }server/v2/api/swagger/config.go (1)
7-7: Remove Unused ImportThe import
"cosmossdk.io/core/server"is not used. Removing it will eliminate linter warnings and keep the code tidy.-import "cosmossdk.io/core/server"🧰 Tools
🪛 golangci-lint (1.62.2)
7-7: "cosmossdk.io/core/server" imported and not used
(typecheck)
🪛 GitHub Actions: Lint
[error] 7-7: "cosmossdk.io/core/server" imported and not used
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
server/v2/api/swagger/config.go(1 hunks)server/v2/api/swagger/handler.go(1 hunks)server/v2/api/swagger/server.go(1 hunks)simapp/v2/app.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
server/v2/api/swagger/handler.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/api/swagger/config.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
simapp/v2/app.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/api/swagger/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 GitHub Actions: Dependency Review
server/v2/api/swagger/handler.go
[error] 10-10: Missing required module: github.com/rakyll/statik/fs. Module needs to be added to the project dependencies.
🪛 golangci-lint (1.62.2)
server/v2/api/swagger/config.go
7-7: "cosmossdk.io/core/server" imported and not used
(typecheck)
simapp/v2/app.go
212-212: app.App.AddServer undefined (type *"cosmossdk.io/runtime/v2".App[T] has no field or method AddServer)
(typecheck)
🪛 GitHub Actions: Lint
server/v2/api/swagger/config.go
[error] 7-7: "cosmossdk.io/core/server" imported and not used
🪛 GitHub Actions: Build SimApp
simapp/v2/app.go
[error] 33-33: Cannot find module providing package cosmossdk.io/client/docs: import lookup disabled by -mod=readonly
🪛 GitHub Actions: v2 core Tests
simapp/v2/app.go
[error] 33-33: Cannot find module providing package cosmossdk.io/client/docs: import lookup disabled by -mod=readonly
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: tests (03)
- GitHub Check: tests (02)
- GitHub Check: tests (00)
- GitHub Check: test-integration
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (1)
server/v2/api/swagger/server.go (1)
14-17: Interface Conformance Looks GoodDeclaring interface compliance at compile-time is a neat way to ensure that your
Server[T]implements the required methods. This is a great practice.
| // Creating and Adding a Swagger Server | ||
| swaggerLogger := logger.With(log.ModuleKey, "swagger") | ||
| swaggerCfg := server.ConfigMap{ | ||
| "swagger": map[string]any{ | ||
| "enable": true, | ||
| "address": "localhost:8080", | ||
| "path": "/swagger/", | ||
| }, | ||
| } | ||
|
|
||
| swaggerServer, err := swaggerv2.New[T]( | ||
| swaggerLogger, | ||
| swaggerCfg, | ||
| swaggerv2.CfgOption(func(cfg *swaggerv2.Config) { | ||
| cfg.SwaggerUI = docs.SwaggerUI | ||
| }), | ||
| ) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create swagger server: %w", err) | ||
| } | ||
|
|
||
| // Adding a Swagger server to the application | ||
| if err := app.App.AddServer(swaggerServer); err != nil { | ||
| return nil, fmt.Errorf("failed to add swagger server: %w", err) | ||
| } |
There was a problem hiding this comment.
Resolve AddServer Method or Provide an Alternative Integration
Static analysis indicates that app.App.AddServer is not defined on the *runtime.App[T]. Ensure you have a corresponding method or consider integrating the Swagger server differently.
- if err := app.App.AddServer(swaggerServer); err != nil {
- return nil, fmt.Errorf("failed to add swagger server: %w", err)
- }
+ // Uncomment or modify once AddServer is correctly implemented on *runtime.App[T].
+ // if err := app.App.AddServer(swaggerServer); err != nil {
+ // return nil, fmt.Errorf("failed to add swagger server: %w", err)
+ // }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Creating and Adding a Swagger Server | |
| swaggerLogger := logger.With(log.ModuleKey, "swagger") | |
| swaggerCfg := server.ConfigMap{ | |
| "swagger": map[string]any{ | |
| "enable": true, | |
| "address": "localhost:8080", | |
| "path": "/swagger/", | |
| }, | |
| } | |
| swaggerServer, err := swaggerv2.New[T]( | |
| swaggerLogger, | |
| swaggerCfg, | |
| swaggerv2.CfgOption(func(cfg *swaggerv2.Config) { | |
| cfg.SwaggerUI = docs.SwaggerUI | |
| }), | |
| ) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to create swagger server: %w", err) | |
| } | |
| // Adding a Swagger server to the application | |
| if err := app.App.AddServer(swaggerServer); err != nil { | |
| return nil, fmt.Errorf("failed to add swagger server: %w", err) | |
| } | |
| // Creating and Adding a Swagger Server | |
| swaggerLogger := logger.With(log.ModuleKey, "swagger") | |
| swaggerCfg := server.ConfigMap{ | |
| "swagger": map[string]any{ | |
| "enable": true, | |
| "address": "localhost:8080", | |
| "path": "/swagger/", | |
| }, | |
| } | |
| swaggerServer, err := swaggerv2.New[T]( | |
| swaggerLogger, | |
| swaggerCfg, | |
| swaggerv2.CfgOption(func(cfg *swaggerv2.Config) { | |
| cfg.SwaggerUI = docs.SwaggerUI | |
| }), | |
| ) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to create swagger server: %w", err) | |
| } | |
| // Adding a Swagger server to the application | |
| // Uncomment or modify once AddServer is correctly implemented on *runtime.App[T]. | |
| // if err := app.App.AddServer(swaggerServer); err != nil { | |
| // return nil, fmt.Errorf("failed to add swagger server: %w", err) | |
| // } |
🧰 Tools
🪛 golangci-lint (1.62.2)
212-212: app.App.AddServer undefined (type *"cosmossdk.io/runtime/v2".App[T] has no field or method AddServer)
(typecheck)
| "strings" | ||
| "time" | ||
|
|
||
| "github.com/rakyll/statik/fs" |
There was a problem hiding this comment.
Add Required Dependency for Statik
The pipeline indicates that the module github.com/rakyll/statik/fs is missing from project dependencies. Update your go.mod by explicitly adding this dependency to avoid build failures.
Would you like assistance creating a diff to add github.com/rakyll/statik to your go.mod file?
🧰 Tools
🪛 GitHub Actions: Dependency Review
[error] 10-10: Missing required module: github.com/rakyll/statik/fs. Module needs to be added to the project dependencies.
|
@julienrbrt made an update, is this doc pretty clear to understand? |
There was a problem hiding this comment.
This isn't how you add server, this should be done in commands.go of simapp/v2
Description
This PR adds Swagger UI support to the server/v2 package, providing an easy way to serve and interact with API documentation. The implementation includes a configurable handler for serving Swagger UI files and proper configuration options.
Closes: #23020
Key changes:
Author Checklist
I have...
[x] included the correct type prefix (feat) in the PR title
[x] confirmed ! in the type prefix if API or client breaking change (not needed)
[x] targeted the correct branch (main)
[x] provided a link to the relevant issue (#23020)
[x] reviewed "Files changed" and left comments if necessary
[x] included the necessary unit and integration tests
[x] added a changelog entry to CHANGELOG.md:
[x] updated the relevant documentation or specification
[x] confirmed all CI checks have passed
Reviewers Checklist
[ ] confirmed the correct type prefix in the PR title
[ ] confirmed all author checklist items have been addressed
[ ] reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage
Changed files:
Summary by CodeRabbit
New Features
Improvements
Technical Enhancements